feat/iii-observability#1678
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request extracts OpenTelemetry, logging, and HTTP tracing functionality into shared Observability packages across Node.js, Python, and Rust, consolidating duplicated code. Each SDK is then refactored to depend on and consume its corresponding Observability package. The release pipeline is updated to build and publish these new packages as standalone artifacts. ChangesCross-SDK Observability Extraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/packages/rust/observability/src/telemetry/mod.rs (1)
402-404:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent
OTEL_REF_COUNTunderflow inshutdown_otelwheninit_otelis a disabled no-op.
OTEL_REF_COUNTstarts atAtomicUsize::new(0), andinit_otelreturnsfalseforenabled: Some(false)before anyfetch_add.shutdown_otelunconditionally doesOTEL_REF_COUNT.fetch_sub(1, ...), so calling it after a disabled init underflows and corrupts later ref-counting (the testinit_otel_with_disabled_config_is_noopcovers this).Suggested fix
pub async fn shutdown_otel() { - let prev = OTEL_REF_COUNT.fetch_sub(1, Ordering::SeqCst); + let prev = match OTEL_REF_COUNT.fetch_update( + Ordering::SeqCst, + Ordering::SeqCst, + |count| count.checked_sub(1), + ) { + Ok(prev) => prev, + Err(_) => { + tracing::debug!("OpenTelemetry not initialized, skipping shutdown"); + return; + } + }; if prev > 1 { tracing::debug!( remaining = prev - 1, "OpenTelemetry still in use, skipping shutdown" ); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/packages/rust/observability/src/telemetry/mod.rs` around lines 402 - 404, shutdown_otel currently unconditionally calls OTEL_REF_COUNT.fetch_sub(1, ...) which underflows if init_otel was a disabled no-op; change shutdown_otel to only decrement the atomic when its current value is > 0 (use AtomicUsize::fetch_update or a load + compare_exchange loop with SeqCst to avoid races) so OTEL_REF_COUNT never goes below zero; update the code in shutdown_otel (referencing OTEL_REF_COUNT and the shutdown_otel function) to perform a conditional decrement and only run the shutdown logic when the decrement succeeds.
🧹 Nitpick comments (2)
sdk/packages/node/observability/package.json (1)
8-8: 💤 Low valueConsider adding a
repositoryfield for better discoverability.Adding a repository field helps with package discoverability and tooling integration. Consider adding:
"repository": { "type": "git", "url": "https://github.com/iii-hq/iii.git", "directory": "sdk/packages/node/observability" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/packages/node/observability/package.json` at line 8, The package.json for the observability package is missing a repository field which reduces discoverability; add a "repository" entry to package.json with type "git", the repo URL "https://github.com/iii-hq/iii.git" and the directory "sdk/packages/node/observability" so tooling and users can locate the source for this package.sdk/packages/python/observability/src/iii_observability/reconnection.py (1)
20-24: ⚡ Quick winValidate reconnection parameter bounds at construction.
ReconnectionConfigaccepts invalid runtime values today (negative delays/retries other than-1, multiplier< 1, jitter outside[0,1]), which can produce pathological retry behavior.Suggested fix
`@dataclass` class ReconnectionConfig: @@ initial_delay_ms: int = 1000 max_delay_ms: int = 30000 backoff_multiplier: float = 2.0 jitter_factor: float = 0.3 max_retries: int = -1 + + def __post_init__(self) -> None: + if self.initial_delay_ms < 0: + raise ValueError("initial_delay_ms must be >= 0") + if self.max_delay_ms < self.initial_delay_ms: + raise ValueError("max_delay_ms must be >= initial_delay_ms") + if self.backoff_multiplier < 1.0: + raise ValueError("backoff_multiplier must be >= 1.0") + if not 0.0 <= self.jitter_factor <= 1.0: + raise ValueError("jitter_factor must be between 0 and 1") + if self.max_retries < -1: + raise ValueError("max_retries must be -1 or >= 0")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/packages/python/observability/src/iii_observability/reconnection.py` around lines 20 - 24, ReconnectionConfig currently allows invalid values; add validation in the ReconnectionConfig constructor or __post_init__ that checks the fields and raises ValueError on bad input: ensure initial_delay_ms and max_delay_ms are integers >= 0 (and optionally require max_delay_ms >= initial_delay_ms), backoff_multiplier is a float >= 1.0, jitter_factor is a float in [0.0, 1.0], and max_retries is an int >= -1 (only -1 allowed as sentinel for unlimited); reference the ReconnectionConfig class and its fields initial_delay_ms, max_delay_ms, backoff_multiplier, jitter_factor, and max_retries when adding these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/packages/node/iii/tests/logger-runtime.test.ts`:
- Around line 51-57: The test registers and looks up a function using the dotted
ID "demo.handler"; update both uses to the engine-standard separator by
replacing "demo.handler" with "demo::handler" in the sdk.registerFunction(...)
call and the sdk.functions.get(...) lookup so the test uses the `::` function ID
format (referencing sdk.registerFunction and sdk.functions.get).
In `@sdk/packages/node/observability/src/http-instrumentation.ts`:
- Around line 22-25: The URL constructor is called on rawUrl without a base
which throws for relative request URLs (e.g. "/orders"); update the parsing
logic around rawUrl/URL so relative URLs are handled safely: detect if rawUrl is
a relative path (no scheme/host or starts with "/") and pass a safe base (for
example a harmless origin like "http://localhost") or wrap new URL(rawUrl) in a
try/catch and fall back to new URL(rawUrl, 'http://localhost'), then compute
method and name from that safe URL; adjust the same guard where URL is used
later (lines 31-40) so executeTracedRequest won’t throw on relative inputs.
In `@sdk/packages/node/observability/src/telemetry-system/log-exporter.ts`:
- Around line 55-57: The pendingExports queue in log-exporter.ts (where it
pushes in the block containing if (state !== 'connected') {
this.pendingExports.push({ logs, callback: resultCallback }) }) must be bounded
to prevent unbounded memory growth; add a MAX_PENDING_EXPORTS constant and
enforce it before push (e.g., if queue.length >= MAX_PENDING_EXPORTS then remove
oldest entry or reject the incoming export), and when dropping an export invoke
the associated callback with an error/Failed result and emit a warning via the
exporter logger so callers are notified; ensure this logic is applied around the
pendingExports push in the same method and that unit tests cover both enqueue
and drop behavior.
In `@sdk/packages/node/observability/src/types.ts`:
- Around line 4-7: The two fields timestamp_unix_nano and
observed_timestamp_unix_nano must not be plain JS numbers; change their type in
sdk/packages/node/observability/src/types.ts to a lossless representation
(prefer a string-based UNIX-ns field, e.g. string) and update
producers/serialization and consumers to treat them as lossless values
(producers emit the nanoseconds as strings, and consumers/console parse to
BigInt before math). Specifically, replace the number type for
timestamp_unix_nano and observed_timestamp_unix_nano, update any serialization
code that writes these fields to emit stringified nanoseconds, and update
consumers (places doing arithmetic like "... / 1_000_000" or
"a.timestamp_unix_nano - b.timestamp_unix_nano") to parse the value to BigInt
and perform integer arithmetic (use BigInt division by 1_000_000n to get
milliseconds) so ordering and timestamps remain exact.
In `@sdk/packages/node/observability/src/utils.ts`:
- Around line 5-10: The safeStringify function can return undefined when
JSON.stringify serializes a top-level undefined/function/symbol, violating its
string return type; update safeStringify (in both implementations) to capture
the result of JSON.stringify(...) and always return a string by coalescing
undefined to a canonical fallback (e.g. '"[unserializable]"' or
'[unserializable]')—use the existing replacer/WeakSet logic as-is, but change
the final return to something like: const out = JSON.stringify(...); return out
?? '"[unserializable]"'.
In `@sdk/packages/python/observability/src/iii_observability/telemetry.py`:
- Around line 330-345: flush_otel currently calls blocking
provider.force_flush() methods directly inside an async function; offload these
synchronous calls to a background thread so the event loop isn't blocked. Change
flush_otel to await asynchronous thread execution for each provider call (e.g.,
using asyncio.to_thread or loop.run_in_executor) when invoking
tracer_provider.force_flush(), _meter_provider.force_flush(), and
_log_provider.force_flush(); preserve the existing hasattr checks
(tracer_provider in trace.get_tracer_provider(), _meter_provider, _log_provider)
and await the threaded calls in sequence (or concurrently with asyncio.gather)
and surface any exceptions as needed.
---
Outside diff comments:
In `@sdk/packages/rust/observability/src/telemetry/mod.rs`:
- Around line 402-404: shutdown_otel currently unconditionally calls
OTEL_REF_COUNT.fetch_sub(1, ...) which underflows if init_otel was a disabled
no-op; change shutdown_otel to only decrement the atomic when its current value
is > 0 (use AtomicUsize::fetch_update or a load + compare_exchange loop with
SeqCst to avoid races) so OTEL_REF_COUNT never goes below zero; update the code
in shutdown_otel (referencing OTEL_REF_COUNT and the shutdown_otel function) to
perform a conditional decrement and only run the shutdown logic when the
decrement succeeds.
---
Nitpick comments:
In `@sdk/packages/node/observability/package.json`:
- Line 8: The package.json for the observability package is missing a repository
field which reduces discoverability; add a "repository" entry to package.json
with type "git", the repo URL "https://github.com/iii-hq/iii.git" and the
directory "sdk/packages/node/observability" so tooling and users can locate the
source for this package.
In `@sdk/packages/python/observability/src/iii_observability/reconnection.py`:
- Around line 20-24: ReconnectionConfig currently allows invalid values; add
validation in the ReconnectionConfig constructor or __post_init__ that checks
the fields and raises ValueError on bad input: ensure initial_delay_ms and
max_delay_ms are integers >= 0 (and optionally require max_delay_ms >=
initial_delay_ms), backoff_multiplier is a float >= 1.0, jitter_factor is a
float in [0.0, 1.0], and max_retries is an int >= -1 (only -1 allowed as
sentinel for unlimited); reference the ReconnectionConfig class and its fields
initial_delay_ms, max_delay_ms, backoff_multiplier, jitter_factor, and
max_retries when adding these checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 56c6ae23-90e0-4c35-82ed-1b5e2392f6a6
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsdk/packages/python/observability/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (101)
.github/workflows/release-iii.ymlCargo.tomlpnpm-workspace.yamlsdk/packages/node/iii/package.jsonsdk/packages/node/iii/src/iii.tssdk/packages/node/iii/src/index.tssdk/packages/node/iii/src/telemetry.tssdk/packages/node/iii/src/types.tssdk/packages/node/iii/src/utils.tssdk/packages/node/iii/tests/baggage-span-processor.test.tssdk/packages/node/iii/tests/bridge-utils.tssdk/packages/node/iii/tests/exports.test.tssdk/packages/node/iii/tests/fetch-instrumentation.test.tssdk/packages/node/iii/tests/logger-runtime.test.tssdk/packages/node/iii/tests/logger.test.tssdk/packages/node/iii/tests/otel-defaults.test.tssdk/packages/node/iii/tests/otel-worker-gauges.test.tssdk/packages/node/iii/tests/payload.test.tssdk/packages/node/iii/tests/span-ops.test.tssdk/packages/node/iii/tests/worker-metrics.test.tssdk/packages/node/iii/vitest.config.tssdk/packages/node/observability/README.mdsdk/packages/node/observability/package.jsonsdk/packages/node/observability/src/http-instrumentation.test.tssdk/packages/node/observability/src/http-instrumentation.tssdk/packages/node/observability/src/index.tssdk/packages/node/observability/src/logger.test.tssdk/packages/node/observability/src/logger.tssdk/packages/node/observability/src/otel-worker-gauges.tssdk/packages/node/observability/src/telemetry-system/baggage-span-processor.tssdk/packages/node/observability/src/telemetry-system/connection.tssdk/packages/node/observability/src/telemetry-system/context.test.tssdk/packages/node/observability/src/telemetry-system/context.tssdk/packages/node/observability/src/telemetry-system/exporters.tssdk/packages/node/observability/src/telemetry-system/fetch-instrumentation.tssdk/packages/node/observability/src/telemetry-system/index.tssdk/packages/node/observability/src/telemetry-system/log-exporter.tssdk/packages/node/observability/src/telemetry-system/metrics-exporter.tssdk/packages/node/observability/src/telemetry-system/payload.tssdk/packages/node/observability/src/telemetry-system/span-exporter.tssdk/packages/node/observability/src/telemetry-system/span-ops.tssdk/packages/node/observability/src/telemetry-system/types.tssdk/packages/node/observability/src/telemetry-system/utils.tssdk/packages/node/observability/src/types.tssdk/packages/node/observability/src/utils.tssdk/packages/node/observability/src/worker-metrics.tssdk/packages/node/observability/tsconfig.jsonsdk/packages/node/observability/tsdown.config.tssdk/packages/node/observability/vitest.config.tssdk/packages/python/iii/pyproject.tomlsdk/packages/python/iii/src/iii/__init__.pysdk/packages/python/iii/src/iii/iii.pysdk/packages/python/iii/src/iii/iii_constants.pysdk/packages/python/iii/tests/test_baggage_span_processor.pysdk/packages/python/iii/tests/test_logger_otel.pysdk/packages/python/iii/tests/test_payload.pysdk/packages/python/iii/tests/test_span_ops.pysdk/packages/python/iii/tests/test_telemetry.pysdk/packages/python/iii/tests/test_telemetry_types.pysdk/packages/python/observability/README.mdsdk/packages/python/observability/pyproject.tomlsdk/packages/python/observability/src/iii_observability/__init__.pysdk/packages/python/observability/src/iii_observability/baggage_span_processor.pysdk/packages/python/observability/src/iii_observability/http_instrumentation.pysdk/packages/python/observability/src/iii_observability/logger.pysdk/packages/python/observability/src/iii_observability/payload.pysdk/packages/python/observability/src/iii_observability/reconnection.pysdk/packages/python/observability/src/iii_observability/span_ops.pysdk/packages/python/observability/src/iii_observability/telemetry.pysdk/packages/python/observability/src/iii_observability/telemetry_exporters.pysdk/packages/python/observability/src/iii_observability/telemetry_types.pysdk/packages/python/observability/tests/__init__.pysdk/packages/python/observability/tests/test_http_instrumentation.pysdk/packages/python/observability/tests/test_telemetry.pysdk/packages/rust/iii/Cargo.tomlsdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/src/lib.rssdk/packages/rust/iii/tests/payload_capture.rssdk/packages/rust/observability/Cargo.tomlsdk/packages/rust/observability/README.mdsdk/packages/rust/observability/src/lib.rssdk/packages/rust/observability/src/logger.rssdk/packages/rust/observability/src/telemetry/baggage_span_processor.rssdk/packages/rust/observability/src/telemetry/connection.rssdk/packages/rust/observability/src/telemetry/context.rssdk/packages/rust/observability/src/telemetry/http_instrumentation.rssdk/packages/rust/observability/src/telemetry/json_serializer.rssdk/packages/rust/observability/src/telemetry/log_exporter.rssdk/packages/rust/observability/src/telemetry/metrics_exporter.rssdk/packages/rust/observability/src/telemetry/mod.rssdk/packages/rust/observability/src/telemetry/otel_worker_gauges.rssdk/packages/rust/observability/src/telemetry/payload.rssdk/packages/rust/observability/src/telemetry/span_exporter.rssdk/packages/rust/observability/src/telemetry/span_ops.rssdk/packages/rust/observability/src/telemetry/types.rssdk/packages/rust/observability/src/telemetry/worker_metrics.rssdk/packages/rust/observability/tests/context_test.rssdk/packages/rust/observability/tests/init_test.rssdk/packages/rust/observability/tests/logger_test.rssdk/packages/rust/observability/tests/payload_test.rssdk/packages/rust/observability/tests/types_test.rs
💤 Files with no reviewable changes (2)
- sdk/packages/node/iii/src/types.ts
- sdk/packages/node/iii/src/utils.ts
Contributor
skill-check — docs10 verified, 341 skipped.
Three for three. Nicely done. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/release-iii.yml (1)
396-403:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
sdk-npmshould pre-build workspace observability dependency inside_npm.yml.
needs: observability-npmonly sequences jobs; it does not build local workspace deps in thesdk-npmjob. Sinceiii-sdkalready needed this prerequisite in CI (Line 548 in.github/workflows/ci.yml), release publish should passpre_build_filtertoo to keep the build deterministic.💡 Suggested workflow fix
sdk-npm: name: SDK Node Publish needs: [setup, create-iii-release, observability-npm] if: ${{ !failure() && !cancelled() }} uses: ./.github/workflows/_npm.yml with: package_path: sdk/packages/node/iii npm_tag: ${{ needs.setup.outputs.npm_tag }} + pre_build_filter: "`@iii-dev/observability`" build_filter: iii-sdk dry_run: ${{ needs.setup.outputs.dry_run == 'true' }} slack_thread_ts: ${{ needs.setup.outputs.slack_ts }} slack_label: SDK Node (npm)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-iii.yml around lines 396 - 403, The sdk-npm release job currently sequences observability-npm but does not pre-build that workspace dependency; update the job that uses ./.github/workflows/_npm.yml (the block with package_path: sdk/packages/node/iii and build_filter: iii-sdk) to pass the pre_build_filter input (e.g., pre_build_filter: observability-npm) so the _npm.yml template will pre-build the observability workspace dependency before publishing.docs/0-11-0/how-to/observability-and-logs.mdx (1)
63-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix remaining Rust
Loggerimport indocs/0-11-0/how-to/observability-and-logs.mdxLine 168 still imports
Loggerfromiii_sdk, leaving conflicting guidance on the same page (the earlier snippet usesiii_observability::Logger).📌 Proposed doc fix
-use iii_sdk::{register_worker, InitOptions, Logger, RegisterFunction}; +use iii_observability::Logger; +use iii_sdk::{register_worker, InitOptions, RegisterFunction};(There are additional stale
iii_sdkLoggerimports in other docs files as well, but this change is required for this page’s consistency.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/0-11-0/how-to/observability-and-logs.mdx` around lines 63 - 71, Update the Rust import so the docs consistently reference iii_observability::Logger rather than the stale iii_sdk import: find the import statement that brings Logger from iii_sdk (the conflicting example on the page) and change it to use iii_observability::Logger, and scan the same docs for other occurrences of iii_sdk::Logger to make the same replacement so all Rust snippets reference the iii_observability Logger type consistently.engine/src/workers/observability/otel.rs (1)
782-810:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Bothexporter path is missingBaggageSpanProcessor, causing mode-specific behavior drift.
ExporterType::Bothsuccess path builds the provider without.with_span_processor(...), unlike the other branches. This drops baggage span processing when OTLP is healthy.Suggested fix
Ok(otlp_exporter) => { init_sdk_span_forwarder(&config.endpoint); // Create tee exporter that sends to both let tee_exporter = TeeSpanExporter::new( otlp_exporter, memory_storage, config.service_name.clone(), ); SdkTracerProvider::builder() + .with_span_processor(iii_observability::BaggageSpanProcessor::default()) .with_batch_exporter(tee_exporter) .with_sampler(sampler) .with_id_generator(RandomIdGenerator::default()) .with_resource(resource) .build() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/src/workers/observability/otel.rs` around lines 782 - 810, In the ExporterType::Both branch you build the SdkTracerProvider without attaching the BaggageSpanProcessor, causing baggage processing to be skipped when OTLP is available; update the ExporterType::Both success path (the block creating TeeSpanExporter and calling SdkTracerProvider::builder()) to also register the BaggageSpanProcessor (the same span processor used in the other branches) via .with_span_processor(...) so baggage is processed consistently (keep init_sdk_span_forwarder, TeeSpanExporter, sampler, id generator, and resource unchanged).
🧹 Nitpick comments (2)
sdk/packages/node/observability/src/http-instrumentation.test.ts (1)
5-15: ⚡ Quick winAdd a regression test for
Requestinputs with existing headers.Current coverage only checks string URL input; it won’t catch header loss when
executeTracedRequestreceives aRequest.✅ Suggested additional test
describe('executeTracedRequest', () => { it('forwards to fetch and returns the response', async () => { @@ }) + + it('preserves existing Request headers when injecting trace headers', async () => { + const originalFetch = globalThis.fetch + const fetchMock = vi.fn().mockResolvedValue(new Response('ok', { status: 200 })) + globalThis.fetch = fetchMock as typeof fetch + try { + const req = new Request('https://example.com/api', { + headers: { authorization: 'Bearer token' }, + }) + await executeTracedRequest(req) + const [, init] = fetchMock.mock.calls[0] + const headers = new Headers(init?.headers) + expect(headers.get('authorization')).toBe('Bearer token') + } finally { + globalThis.fetch = originalFetch + } + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/packages/node/observability/src/http-instrumentation.test.ts` around lines 5 - 15, Add a regression test in http-instrumentation.test.ts that calls executeTracedRequest with a Request instance that already contains headers (e.g., an Authorization or custom header) to ensure headers are preserved; mock globalThis.fetch (like existing tests) to capture the incoming Request argument and assert the header is present on the request passed to fetch, and that the response from fetch is forwarded unchanged (status/body). Reference executeTracedRequest and the existing test pattern for restoring globalThis.fetch when implementing the new test.sdk/packages/python/observability/tests/test_http_instrumentation.py (1)
6-22: ⚡ Quick winTighten assertions to validate instrumentation behavior (not only HTTP responses).
Line 17’s test name implies span error-status behavior, but it only checks
res.status_code. Please either assert instrumentation side-effects (e.g.,traceparentheader presence via mocked requests) or rename tests to reflect response-only validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/packages/python/observability/tests/test_http_instrumentation.py` around lines 6 - 22, The test test_execute_traced_request_records_error_status currently only asserts HTTP response code; update it to validate instrumentation side-effects from execute_traced_request by asserting that the outgoing request carried tracing headers (e.g., presence of "traceparent" or "tracestate" on the built request or in httpx_mock.request_history) or by inspecting recorded spans from the test tracer/exporter to ensure the span has error status for 500 responses; modify the test to either (a) check httpx_mock.request_history[0].headers contains "traceparent" and that a span/emitted event reflects error status, or (b) rename the test to indicate it only validates HTTP response if you prefer not to assert instrumentation. Ensure you reference execute_traced_request and test_execute_traced_request_records_error_status when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/packages/node/observability/src/telemetry-system/connection.ts`:
- Around line 117-120: The loop that invokes each registered failure handler
(the this.onFailedCallbacks iteration) must isolate exceptions so one bad
callback doesn't stop the rest; wrap each cb() call in a try/catch and on error
log the exception (use an existing logger if available, e.g. this.logger.error,
or console.error) and continue to the next callback. Apply the same change to
the other place where you iterate this.onFailedCallbacks (the second draining
loop) so both notification sites catch and log individual callback errors and
keep draining exporters.
---
Outside diff comments:
In @.github/workflows/release-iii.yml:
- Around line 396-403: The sdk-npm release job currently sequences
observability-npm but does not pre-build that workspace dependency; update the
job that uses ./.github/workflows/_npm.yml (the block with package_path:
sdk/packages/node/iii and build_filter: iii-sdk) to pass the pre_build_filter
input (e.g., pre_build_filter: observability-npm) so the _npm.yml template will
pre-build the observability workspace dependency before publishing.
In `@docs/0-11-0/how-to/observability-and-logs.mdx`:
- Around line 63-71: Update the Rust import so the docs consistently reference
iii_observability::Logger rather than the stale iii_sdk import: find the import
statement that brings Logger from iii_sdk (the conflicting example on the page)
and change it to use iii_observability::Logger, and scan the same docs for other
occurrences of iii_sdk::Logger to make the same replacement so all Rust snippets
reference the iii_observability Logger type consistently.
In `@engine/src/workers/observability/otel.rs`:
- Around line 782-810: In the ExporterType::Both branch you build the
SdkTracerProvider without attaching the BaggageSpanProcessor, causing baggage
processing to be skipped when OTLP is available; update the ExporterType::Both
success path (the block creating TeeSpanExporter and calling
SdkTracerProvider::builder()) to also register the BaggageSpanProcessor (the
same span processor used in the other branches) via .with_span_processor(...) so
baggage is processed consistently (keep init_sdk_span_forwarder,
TeeSpanExporter, sampler, id generator, and resource unchanged).
---
Nitpick comments:
In `@sdk/packages/node/observability/src/http-instrumentation.test.ts`:
- Around line 5-15: Add a regression test in http-instrumentation.test.ts that
calls executeTracedRequest with a Request instance that already contains headers
(e.g., an Authorization or custom header) to ensure headers are preserved; mock
globalThis.fetch (like existing tests) to capture the incoming Request argument
and assert the header is present on the request passed to fetch, and that the
response from fetch is forwarded unchanged (status/body). Reference
executeTracedRequest and the existing test pattern for restoring
globalThis.fetch when implementing the new test.
In `@sdk/packages/python/observability/tests/test_http_instrumentation.py`:
- Around line 6-22: The test test_execute_traced_request_records_error_status
currently only asserts HTTP response code; update it to validate instrumentation
side-effects from execute_traced_request by asserting that the outgoing request
carried tracing headers (e.g., presence of "traceparent" or "tracestate" on the
built request or in httpx_mock.request_history) or by inspecting recorded spans
from the test tracer/exporter to ensure the span has error status for 500
responses; modify the test to either (a) check
httpx_mock.request_history[0].headers contains "traceparent" and that a
span/emitted event reflects error status, or (b) rename the test to indicate it
only validates HTTP response if you prefer not to assert instrumentation. Ensure
you reference execute_traced_request and
test_execute_traced_request_records_error_status when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 62d3dca0-3951-4a06-bd44-d02127c4e78a
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsdk/packages/python/iii-example/uv.lockis excluded by!**/*.locksdk/packages/python/iii/uv.lockis excluded by!**/*.locksdk/packages/python/observability/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (132)
.github/workflows/ci.yml.github/workflows/release-iii.ymlCargo.tomlconsole/packages/console-rust/Cargo.tomlconsole/packages/console-rust/src/main.rscrates/iii-worker/Cargo.tomlcrates/iii-worker/src/cli/worker_manager_daemon.rscrates/iii-worker/src/sandbox_daemon/mod.rsdocs/0-11-0/how-to/observability-and-logs.mdxengine/Cargo.tomlengine/src/workers/observability/otel.rspnpm-workspace.yamlsdk/packages/node/iii/package.jsonsdk/packages/node/iii/src/iii.tssdk/packages/node/iii/src/index.tssdk/packages/node/iii/src/types.tssdk/packages/node/iii/src/utils.tssdk/packages/node/iii/tests/baggage-span-processor.test.tssdk/packages/node/iii/tests/bridge-utils.tssdk/packages/node/iii/tests/exports.test.tssdk/packages/node/iii/tests/fetch-instrumentation.test.tssdk/packages/node/iii/tests/logger-runtime.test.tssdk/packages/node/iii/tests/logger.test.tssdk/packages/node/iii/tests/otel-defaults.test.tssdk/packages/node/iii/tests/otel-worker-gauges.test.tssdk/packages/node/iii/tests/payload.test.tssdk/packages/node/iii/tests/span-ops.test.tssdk/packages/node/iii/tests/worker-metrics.test.tssdk/packages/node/iii/tsdown.config.tssdk/packages/node/iii/vitest.config.tssdk/packages/node/observability/README.mdsdk/packages/node/observability/package.jsonsdk/packages/node/observability/src/http-instrumentation.test.tssdk/packages/node/observability/src/http-instrumentation.tssdk/packages/node/observability/src/index.tssdk/packages/node/observability/src/logger.test.tssdk/packages/node/observability/src/logger.tssdk/packages/node/observability/src/otel-worker-gauges.tssdk/packages/node/observability/src/telemetry-system/baggage-span-processor.tssdk/packages/node/observability/src/telemetry-system/connection.tssdk/packages/node/observability/src/telemetry-system/context.test.tssdk/packages/node/observability/src/telemetry-system/context.tssdk/packages/node/observability/src/telemetry-system/exporters.tssdk/packages/node/observability/src/telemetry-system/fetch-instrumentation.tssdk/packages/node/observability/src/telemetry-system/index.tssdk/packages/node/observability/src/telemetry-system/log-exporter.tssdk/packages/node/observability/src/telemetry-system/metrics-exporter.tssdk/packages/node/observability/src/telemetry-system/payload.tssdk/packages/node/observability/src/telemetry-system/span-exporter.tssdk/packages/node/observability/src/telemetry-system/span-ops.tssdk/packages/node/observability/src/telemetry-system/types.tssdk/packages/node/observability/src/telemetry-system/utils.tssdk/packages/node/observability/src/types.tssdk/packages/node/observability/src/utils.tssdk/packages/node/observability/src/worker-metrics.tssdk/packages/node/observability/tsconfig.jsonsdk/packages/node/observability/tsdown.config.tssdk/packages/node/observability/vitest.config.tssdk/packages/python/iii-example/pyproject.tomlsdk/packages/python/iii-example/src/hooks.pysdk/packages/python/iii-example/src/state.pysdk/packages/python/iii-example/src/stream.pysdk/packages/python/iii/pyproject.tomlsdk/packages/python/iii/src/iii/__init__.pysdk/packages/python/iii/src/iii/iii.pysdk/packages/python/iii/src/iii/iii_constants.pysdk/packages/python/iii/tests/test_baggage_span_processor.pysdk/packages/python/iii/tests/test_context_propagation.pysdk/packages/python/iii/tests/test_hold_process.pysdk/packages/python/iii/tests/test_http_external_functions_integration.pysdk/packages/python/iii/tests/test_iii_registration_dedup.pysdk/packages/python/iii/tests/test_init_api.pysdk/packages/python/iii/tests/test_logger_function_ids.pysdk/packages/python/iii/tests/test_logger_otel.pysdk/packages/python/iii/tests/test_payload.pysdk/packages/python/iii/tests/test_register_function_args.pysdk/packages/python/iii/tests/test_span_ops.pysdk/packages/python/iii/tests/test_sync_api.pysdk/packages/python/iii/tests/test_telemetry.pysdk/packages/python/iii/tests/test_telemetry_exporters.pysdk/packages/python/iii/tests/test_telemetry_types.pysdk/packages/python/iii/tests/test_trace_helpers.pysdk/packages/python/observability/README.mdsdk/packages/python/observability/pyproject.tomlsdk/packages/python/observability/src/iii_observability/__init__.pysdk/packages/python/observability/src/iii_observability/baggage_span_processor.pysdk/packages/python/observability/src/iii_observability/http_instrumentation.pysdk/packages/python/observability/src/iii_observability/logger.pysdk/packages/python/observability/src/iii_observability/payload.pysdk/packages/python/observability/src/iii_observability/py.typedsdk/packages/python/observability/src/iii_observability/reconnection.pysdk/packages/python/observability/src/iii_observability/span_ops.pysdk/packages/python/observability/src/iii_observability/telemetry.pysdk/packages/python/observability/src/iii_observability/telemetry_exporters.pysdk/packages/python/observability/src/iii_observability/telemetry_types.pysdk/packages/python/observability/tests/__init__.pysdk/packages/python/observability/tests/test_http_instrumentation.pysdk/packages/python/observability/tests/test_telemetry.pysdk/packages/rust/iii-example/Cargo.tomlsdk/packages/rust/iii-example/src/http_example.rssdk/packages/rust/iii-example/src/logger_example.rssdk/packages/rust/iii-example/src/main.rssdk/packages/rust/iii/Cargo.tomlsdk/packages/rust/iii/README.mdsdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/src/lib.rssdk/packages/rust/iii/tests/init_api.rssdk/packages/rust/iii/tests/payload_capture.rssdk/packages/rust/iii/tests/span_ops_api.rssdk/packages/rust/observability/Cargo.tomlsdk/packages/rust/observability/README.mdsdk/packages/rust/observability/src/lib.rssdk/packages/rust/observability/src/logger.rssdk/packages/rust/observability/src/telemetry/baggage_span_processor.rssdk/packages/rust/observability/src/telemetry/connection.rssdk/packages/rust/observability/src/telemetry/context.rssdk/packages/rust/observability/src/telemetry/http_instrumentation.rssdk/packages/rust/observability/src/telemetry/json_serializer.rssdk/packages/rust/observability/src/telemetry/log_exporter.rssdk/packages/rust/observability/src/telemetry/metrics_exporter.rssdk/packages/rust/observability/src/telemetry/mod.rssdk/packages/rust/observability/src/telemetry/otel_worker_gauges.rssdk/packages/rust/observability/src/telemetry/payload.rssdk/packages/rust/observability/src/telemetry/span_exporter.rssdk/packages/rust/observability/src/telemetry/span_ops.rssdk/packages/rust/observability/src/telemetry/types.rssdk/packages/rust/observability/src/telemetry/worker_metrics.rssdk/packages/rust/observability/tests/context_test.rssdk/packages/rust/observability/tests/init_test.rssdk/packages/rust/observability/tests/logger_test.rssdk/packages/rust/observability/tests/payload_test.rssdk/packages/rust/observability/tests/types_test.rs
💤 Files with no reviewable changes (3)
- sdk/packages/node/iii/src/utils.ts
- sdk/packages/node/iii/src/types.ts
- sdk/packages/node/iii/tests/exports.test.ts
✅ Files skipped from review due to trivial changes (28)
- sdk/packages/rust/observability/README.md
- crates/iii-worker/Cargo.toml
- sdk/packages/python/iii/tests/test_trace_helpers.py
- sdk/packages/rust/iii-example/src/logger_example.rs
- console/packages/console-rust/Cargo.toml
- engine/Cargo.toml
- sdk/packages/node/iii/tests/worker-metrics.test.ts
- sdk/packages/rust/observability/src/telemetry/http_instrumentation.rs
- sdk/packages/rust/iii/tests/init_api.rs
- sdk/packages/rust/iii-example/Cargo.toml
- sdk/packages/node/observability/vitest.config.ts
- sdk/packages/node/observability/package.json
- sdk/packages/python/iii/tests/test_logger_function_ids.py
- sdk/packages/rust/observability/src/telemetry/mod.rs
- sdk/packages/rust/iii/README.md
- sdk/packages/python/observability/README.md
- sdk/packages/python/iii-example/src/hooks.py
- sdk/packages/rust/observability/Cargo.toml
- sdk/packages/node/iii/tests/payload.test.ts
- sdk/packages/python/iii/tests/test_telemetry_exporters.py
- sdk/packages/python/iii/tests/test_baggage_span_processor.py
- sdk/packages/python/iii/tests/test_init_api.py
- sdk/packages/python/observability/pyproject.toml
- sdk/packages/python/iii/tests/test_payload.py
- sdk/packages/node/observability/README.md
- sdk/packages/rust/observability/src/logger.rs
- sdk/packages/python/iii/src/iii/iii.py
- sdk/packages/python/iii/tests/test_telemetry_types.py
Copies payload.rs, span_ops.rs, and baggage_span_processor.rs verbatim from iii-sdk into the iii-observability crate and re-exports all public symbols at the crate root. Adds integration test payload_test.rs (TDD).
…d_request Move all OTel lifecycle functions (init_otel, shutdown_otel, flush_otel, with_span, run_in_span) and their required internal modules (connection, json_serializer, log_exporter, metrics_exporter, span_exporter, otel_worker_gauges, worker_metrics, http_instrumentation) from iii-sdk into iii-observability. Add public re-exports in lib.rs and add futures-util, tokio-tungstenite, and uuid as direct dependencies.
…compat Delete iii-sdk's own telemetry/ and logger.rs; add iii-observability as a workspace dep instead. All OTel symbols are re-exported from the new crate at the iii-sdk crate root for back-compat. iii.rs internal usages of opentelemetry:: are routed through iii_observability::opentelemetry. Test file payload_capture.rs updated to use the flat re-export path.
…rker-gauges Copy telemetry-system folder, worker-metrics.ts, and otel-worker-gauges.ts from iii-sdk into @iii-dev/observability. Add OtelLogEvent to local types.ts, safeStringify to local utils.ts, and ws dependency. All imports self-contained.
…cy with other exporters
3 tasks
guibeira
added a commit
that referenced
this pull request
May 25, 2026
These doc/skill updates belong with the observability work in PR #1678 (feat/new-libs); they were carried here while align-rust-sdk was rebased on top. Revert them here so this PR stays focused on the Rust SDK API reshape; the same changes are being applied on feat/new-libs.
…kills Move the doc/skill updates that were carried on align-rust-sdk into this branch so they ship alongside the observability work. - Regenerate sdk-node / sdk-python / sdk-rust API references for the new Rust register_function shape (`register_function(id, RegisterFunction::…)`, 3 constructors, IIIError handler errors). - Add the migration changelog entry under docs/changelog/0-11-0/. - Update docs/0-11-0/scripts/generate-api-docs.mts + parsers/parse-rustdoc.mts to scan FunctionDoc descriptions/examples and allowlist RegisterFunctionOptions in the rustdoc parser. - Refresh skills/iii-rust-sdk/SKILL.md and skills/references/http-invoked-functions.rs to reflect the new Rust call shape.
… to index - Move docs/changelog/0-11-0/align-rust-register-function-with-signature.mdx to docs/changelog/0-14-0/. The change ships in 0.14, not 0.11. - Add a 0.14.0 entry in docs/changelog/index.mdx linking to the migration page. - Register the new page under the "Changelog" tab in docs/docs.json.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
sdk/packages/node/observability/src/types.ts (1)
4-6:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUse a lossless type for Unix-nanosecond timestamps.
timestamp_unix_nanoandobserved_timestamp_unix_nanoasnumberare precision-lossy in JS and can produce incorrect ordering/time calculations. This concern was already raised earlier and still applies.💡 Proposed contract fix
export type OtelLogEvent = { /** Timestamp in Unix nanoseconds */ - timestamp_unix_nano: number + timestamp_unix_nano: string /** Observed timestamp in Unix nanoseconds */ - observed_timestamp_unix_nano: number + observed_timestamp_unix_nano: string#!/bin/bash # Verify precision-risk definitions and downstream arithmetic usage. # Expected: if these fields are typed as number and used in numeric math/sort, # the precision-loss concern is confirmed. rg -n --type=ts -C2 '\b(timestamp_unix_nano|observed_timestamp_unix_nano)\b' rg -nP --type=ts -C2 '\b[a-zA-Z0-9_.]*(timestamp_unix_nano|observed_timestamp_unix_nano)\b\s*[-+*/]'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/packages/node/observability/src/types.ts` around lines 4 - 6, The timestamp_unix_nano and observed_timestamp_unix_nano fields in types.ts are currently typed as number (lossy); change their types to string (or bigint if your transport supports it) to preserve full Unix-nanosecond precision in the contract, update any consuming code that performs numeric math or sorting on these fields (e.g., convert to BigInt via BigInt(value) or use string-based comparison/parsing) and run the suggested rg checks to find and fix all downstream arithmetic usages of timestamp_unix_nano/observed_timestamp_unix_nano so no precision-lossy number ops remain.sdk/packages/node/observability/src/telemetry-system/connection.ts (1)
117-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIsolate
onFailedcallback exceptions at both notification sites.A throwing callback on Line 119 or Line 183 can stop remaining callbacks, which may leave exporter drains incomplete after terminal connection failure.
Proposed fix
// Notify failed callbacks so exporters can drain their own queues for (const cb of this.onFailedCallbacks) { - cb() + try { + cb() + } catch (err) { + console.error('[OTel] onFailed callback threw:', err) + } } @@ onFailed(callback: () => void): void { this.onFailedCallbacks.push(callback) if (this.state === 'failed') { - callback() + try { + callback() + } catch (err) { + console.error('[OTel] onFailed callback threw:', err) + } } }Also applies to: 180-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/packages/node/observability/src/telemetry-system/connection.ts` around lines 117 - 120, The loops that notify onFailed callbacks (iterating this.onFailedCallbacks and invoking cb()) must isolate exceptions so one bad callback cannot stop the rest; wrap each cb() invocation in its own try/catch, catch and log the error (using the available logger or console) and continue to the next callback; apply this change at both notification sites that iterate this.onFailedCallbacks (the two places where callbacks are invoked) so all callbacks are guaranteed to run even if one throws.
🧹 Nitpick comments (2)
sdk/packages/rust/observability/src/lib.rs (1)
2-2: ⚡ Quick winAvoid hard-coded crate version drift.
Line 2 hard-codes the version string. Prefer deriving it from Cargo metadata to keep it always in sync.
Suggested fix
-pub const VERSION: &str = "0.13.0-next.1"; +pub const VERSION: &str = env!("CARGO_PKG_VERSION");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/packages/rust/observability/src/lib.rs` at line 2, Replace the hard-coded VERSION constant with a compile-time value derived from Cargo metadata: update the pub const VERSION symbol to use the package version provided by Cargo (e.g., via the env! macro with CARGO_PKG_VERSION) so the crate version is always in sync with Cargo.toml when building.sdk/packages/rust/iii/src/iii.rs (1)
46-47: 💤 Low valueConsider using the
telemetryalias consistently throughout the file.The file defines
use iii_observability as telemetry;at line 46, but then:
- Line 318 creates a redundant local alias
use iii_observability as context;- Lines 1611-1654 use
iii_observability::directly instead of thetelemetry::aliasUsing the alias consistently would improve readability and make future refactoring easier.
Also applies to: 318-319, 1611-1654
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/packages/rust/iii/src/iii.rs` around lines 46 - 47, The file already aliases iii_observability as telemetry; make that alias the single source of truth by removing the redundant local alias (the use iii_observability as context) and replacing all direct uses of the crate path (iii_observability::...) with the telemetry:: alias; update any function calls, types, and constants referenced with iii_observability:: to telemetry:: (e.g., OtelConfig -> telemetry::OtelConfig, any telemetry::tracing or telemetry::context usages) so the file consistently uses telemetry throughout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/0-11-0/api-reference/sdk-node.mdx`:
- Line 190: Replace the incorrect cron config key `schedule` with the
engine-standard `expression` in the examples: update the object shown as
`config: { schedule: '*/5 * * * *' }` to `config: { expression: '*/5 * * * *' }`
and make the same change for the other occurrences referenced around lines
217-223 so all cron trigger examples use `expression` (not `schedule` or `cron`)
to match the engine standard.
In `@docs/0-11-0/api-reference/sdk-rust.mdx`:
- Line 209: Update the register_trigger example so it constructs a
RegisterTriggerInput using exactly the three documented fields (trigger_type,
function_id, config) — remove the extraneous metadata field (e.g., `metadata:
None`) and ensure the example variable passed to register_trigger matches the
documented RegisterTriggerInput shape.
- Line 103: The docs list `RegisterFunction::new_async` twice; remove the
duplicate and replace it with the actual distinct constructor/helper intended
(check the `RegisterFunction` impl for available constructors such as
`RegisterFunction::new_streaming`, `RegisterFunction::new_sync`, or any other
public helper) so the line reads something like `RegisterFunction::new`,
`RegisterFunction::new_async`, `RegisterFunction::<correct_constructor>`, or
`RegisterFunction::http` as appropriate; ensure you keep
`RegisterFunction::new`, `RegisterFunction::new_async`, and
`RegisterFunction::http` and update the duplicated entry to the correct symbol.
In `@docs/understanding-iii/triggers.mdx.skill.md`:
- Around line 99-101: The fenced code blocks containing the example message
starting with "[iii] Trigger registration failed for "1c1b…" (http): Trigger
type "http" not found — worker iii-http is missing. Run: iii worker add
iii-http" (and the similar block at lines 105-107) are missing language tags;
update both triple-backtick fences to begin with ```text (or another explicit
language) to satisfy markdownlint MD040.
In `@engine/src/workers/observability/otel.rs`:
- Around line 824-825: The ExporterType::Both branch is missing the
BaggageSpanProcessor that the fallback path adds; update the Both-path
tracer/metric exporter builder to include
.with_span_processor(iii_observability::BaggageSpanProcessor::default()) in the
same place the fallback uses it (i.e., add the call into the builder chain that
currently calls .with_simple_exporter(exporter) in the Both branch) so baggage
propagation behavior is consistent across success and fallback paths.
In `@sdk/packages/node/observability/src/http-instrumentation.ts`:
- Line 53: executeTracedRequest currently drops headers when the caller passes a
Request object because it blindly does const headers = new
Headers(init?.headers) and then fetch(input, { ...init, headers }); update the
function to detect when input is a Request (e.g., input instanceof Request),
construct headers by starting from the Request's existing headers and then
merge/override with init?.headers (or vice‑versa depending on intended
precedence) so both sets are preserved before calling fetch; reference the
headers variable and the fetch(input, { ...init, headers }) call to locate where
to merge the headers.
In `@skills/iii-rust-sdk/SKILL.md`:
- Around line 18-21: The Cargo.toml snippet pins iii-sdk to an outdated version
("0.10"); update the example dependency line to match the SDK manifest version
used in this PR (iii-sdk = { version = "0.13.0", features = ["otel"] }) so
docs/skill shows the correct API surface — change the version string in the
snippet where `iii-sdk = { version = "...", features = ["otel"] }` is shown to
the current package version.
In `@skills/references/http-invoked-functions.rs`:
- Line 12: The import currently pulls Logger from iii_sdk but Logger has been
moved; update the use statement to import Logger from the new crate surface
(iii_observability) instead of iii_sdk so symbols like Logger used in this file
(e.g., in InitOptions or any logging calls) resolve correctly; modify the
existing use line that mentions Logger to reference iii_observability::Logger
while leaving the rest of the iii_sdk imports (HttpAuthConfig,
HttpInvocationConfig, IIITrigger, RegisterFunction, TriggerAction,
TriggerRequest, builtin_triggers::*, protocol::HttpMethod, register_worker)
unchanged.
---
Duplicate comments:
In `@sdk/packages/node/observability/src/telemetry-system/connection.ts`:
- Around line 117-120: The loops that notify onFailed callbacks (iterating
this.onFailedCallbacks and invoking cb()) must isolate exceptions so one bad
callback cannot stop the rest; wrap each cb() invocation in its own try/catch,
catch and log the error (using the available logger or console) and continue to
the next callback; apply this change at both notification sites that iterate
this.onFailedCallbacks (the two places where callbacks are invoked) so all
callbacks are guaranteed to run even if one throws.
In `@sdk/packages/node/observability/src/types.ts`:
- Around line 4-6: The timestamp_unix_nano and observed_timestamp_unix_nano
fields in types.ts are currently typed as number (lossy); change their types to
string (or bigint if your transport supports it) to preserve full
Unix-nanosecond precision in the contract, update any consuming code that
performs numeric math or sorting on these fields (e.g., convert to BigInt via
BigInt(value) or use string-based comparison/parsing) and run the suggested rg
checks to find and fix all downstream arithmetic usages of
timestamp_unix_nano/observed_timestamp_unix_nano so no precision-lossy number
ops remain.
---
Nitpick comments:
In `@sdk/packages/rust/iii/src/iii.rs`:
- Around line 46-47: The file already aliases iii_observability as telemetry;
make that alias the single source of truth by removing the redundant local alias
(the use iii_observability as context) and replacing all direct uses of the
crate path (iii_observability::...) with the telemetry:: alias; update any
function calls, types, and constants referenced with iii_observability:: to
telemetry:: (e.g., OtelConfig -> telemetry::OtelConfig, any telemetry::tracing
or telemetry::context usages) so the file consistently uses telemetry
throughout.
In `@sdk/packages/rust/observability/src/lib.rs`:
- Line 2: Replace the hard-coded VERSION constant with a compile-time value
derived from Cargo metadata: update the pub const VERSION symbol to use the
package version provided by Cargo (e.g., via the env! macro with
CARGO_PKG_VERSION) so the crate version is always in sync with Cargo.toml when
building.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e1458ec2-07f9-4306-a0a4-ef424e16a74b
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsdk/packages/python/iii-example/uv.lockis excluded by!**/*.locksdk/packages/python/iii/uv.lockis excluded by!**/*.locksdk/packages/python/observability/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (140)
.github/workflows/ci.yml.github/workflows/release-iii.ymlCargo.tomlconsole/packages/console-rust/Cargo.tomlconsole/packages/console-rust/src/main.rscrates/iii-worker/Cargo.tomlcrates/iii-worker/src/cli/worker_manager_daemon.rscrates/iii-worker/src/sandbox_daemon/mod.rsdocs/0-11-0/api-reference/sdk-node.mdxdocs/0-11-0/api-reference/sdk-python.mdxdocs/0-11-0/api-reference/sdk-rust.mdxdocs/0-11-0/scripts/generate-api-docs.mtsdocs/0-11-0/scripts/parsers/parse-rustdoc.mtsdocs/changelog/0-11-0/align-rust-register-function-with-signature.mdxdocs/understanding-iii/triggers.mdx.skill.mdengine/Cargo.tomlengine/src/workers/observability/otel.rspnpm-workspace.yamlsdk/packages/node/iii/package.jsonsdk/packages/node/iii/src/iii.tssdk/packages/node/iii/src/index.tssdk/packages/node/iii/src/types.tssdk/packages/node/iii/src/utils.tssdk/packages/node/iii/tests/baggage-span-processor.test.tssdk/packages/node/iii/tests/bridge-utils.tssdk/packages/node/iii/tests/exports.test.tssdk/packages/node/iii/tests/fetch-instrumentation.test.tssdk/packages/node/iii/tests/logger-runtime.test.tssdk/packages/node/iii/tests/logger.test.tssdk/packages/node/iii/tests/otel-defaults.test.tssdk/packages/node/iii/tests/otel-worker-gauges.test.tssdk/packages/node/iii/tests/payload.test.tssdk/packages/node/iii/tests/span-ops.test.tssdk/packages/node/iii/tests/worker-metrics.test.tssdk/packages/node/iii/tsdown.config.tssdk/packages/node/iii/vitest.config.tssdk/packages/node/observability/README.mdsdk/packages/node/observability/package.jsonsdk/packages/node/observability/src/http-instrumentation.test.tssdk/packages/node/observability/src/http-instrumentation.tssdk/packages/node/observability/src/index.tssdk/packages/node/observability/src/logger.test.tssdk/packages/node/observability/src/logger.tssdk/packages/node/observability/src/otel-worker-gauges.tssdk/packages/node/observability/src/telemetry-system/baggage-span-processor.tssdk/packages/node/observability/src/telemetry-system/connection.tssdk/packages/node/observability/src/telemetry-system/context.test.tssdk/packages/node/observability/src/telemetry-system/context.tssdk/packages/node/observability/src/telemetry-system/exporters.tssdk/packages/node/observability/src/telemetry-system/fetch-instrumentation.tssdk/packages/node/observability/src/telemetry-system/index.tssdk/packages/node/observability/src/telemetry-system/log-exporter.tssdk/packages/node/observability/src/telemetry-system/metrics-exporter.tssdk/packages/node/observability/src/telemetry-system/payload.tssdk/packages/node/observability/src/telemetry-system/span-exporter.tssdk/packages/node/observability/src/telemetry-system/span-ops.tssdk/packages/node/observability/src/telemetry-system/types.tssdk/packages/node/observability/src/telemetry-system/utils.tssdk/packages/node/observability/src/types.tssdk/packages/node/observability/src/utils.tssdk/packages/node/observability/src/worker-metrics.tssdk/packages/node/observability/tsconfig.jsonsdk/packages/node/observability/tsdown.config.tssdk/packages/node/observability/vitest.config.tssdk/packages/python/iii-example/pyproject.tomlsdk/packages/python/iii-example/src/hooks.pysdk/packages/python/iii-example/src/state.pysdk/packages/python/iii-example/src/stream.pysdk/packages/python/iii/pyproject.tomlsdk/packages/python/iii/src/iii/__init__.pysdk/packages/python/iii/src/iii/iii.pysdk/packages/python/iii/src/iii/iii_constants.pysdk/packages/python/iii/tests/test_baggage_span_processor.pysdk/packages/python/iii/tests/test_context_propagation.pysdk/packages/python/iii/tests/test_hold_process.pysdk/packages/python/iii/tests/test_http_external_functions_integration.pysdk/packages/python/iii/tests/test_iii_registration_dedup.pysdk/packages/python/iii/tests/test_init_api.pysdk/packages/python/iii/tests/test_logger_function_ids.pysdk/packages/python/iii/tests/test_logger_otel.pysdk/packages/python/iii/tests/test_payload.pysdk/packages/python/iii/tests/test_register_function_args.pysdk/packages/python/iii/tests/test_span_ops.pysdk/packages/python/iii/tests/test_sync_api.pysdk/packages/python/iii/tests/test_telemetry.pysdk/packages/python/iii/tests/test_telemetry_exporters.pysdk/packages/python/iii/tests/test_telemetry_types.pysdk/packages/python/iii/tests/test_trace_helpers.pysdk/packages/python/observability/README.mdsdk/packages/python/observability/pyproject.tomlsdk/packages/python/observability/src/iii_observability/__init__.pysdk/packages/python/observability/src/iii_observability/baggage_span_processor.pysdk/packages/python/observability/src/iii_observability/http_instrumentation.pysdk/packages/python/observability/src/iii_observability/logger.pysdk/packages/python/observability/src/iii_observability/payload.pysdk/packages/python/observability/src/iii_observability/py.typedsdk/packages/python/observability/src/iii_observability/reconnection.pysdk/packages/python/observability/src/iii_observability/span_ops.pysdk/packages/python/observability/src/iii_observability/telemetry.pysdk/packages/python/observability/src/iii_observability/telemetry_exporters.pysdk/packages/python/observability/src/iii_observability/telemetry_types.pysdk/packages/python/observability/tests/__init__.pysdk/packages/python/observability/tests/test_http_instrumentation.pysdk/packages/python/observability/tests/test_telemetry.pysdk/packages/rust/iii-example/Cargo.tomlsdk/packages/rust/iii-example/src/http_example.rssdk/packages/rust/iii-example/src/logger_example.rssdk/packages/rust/iii-example/src/main.rssdk/packages/rust/iii/Cargo.tomlsdk/packages/rust/iii/README.mdsdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/src/lib.rssdk/packages/rust/iii/tests/init_api.rssdk/packages/rust/iii/tests/payload_capture.rssdk/packages/rust/iii/tests/span_ops_api.rssdk/packages/rust/observability/Cargo.tomlsdk/packages/rust/observability/README.mdsdk/packages/rust/observability/src/lib.rssdk/packages/rust/observability/src/logger.rssdk/packages/rust/observability/src/telemetry/baggage_span_processor.rssdk/packages/rust/observability/src/telemetry/connection.rssdk/packages/rust/observability/src/telemetry/context.rssdk/packages/rust/observability/src/telemetry/http_instrumentation.rssdk/packages/rust/observability/src/telemetry/json_serializer.rssdk/packages/rust/observability/src/telemetry/log_exporter.rssdk/packages/rust/observability/src/telemetry/metrics_exporter.rssdk/packages/rust/observability/src/telemetry/mod.rssdk/packages/rust/observability/src/telemetry/otel_worker_gauges.rssdk/packages/rust/observability/src/telemetry/payload.rssdk/packages/rust/observability/src/telemetry/span_exporter.rssdk/packages/rust/observability/src/telemetry/span_ops.rssdk/packages/rust/observability/src/telemetry/types.rssdk/packages/rust/observability/src/telemetry/worker_metrics.rssdk/packages/rust/observability/tests/context_test.rssdk/packages/rust/observability/tests/init_test.rssdk/packages/rust/observability/tests/logger_test.rssdk/packages/rust/observability/tests/payload_test.rssdk/packages/rust/observability/tests/types_test.rsskills/iii-rust-sdk/SKILL.mdskills/references/http-invoked-functions.rs
💤 Files with no reviewable changes (3)
- sdk/packages/node/iii/src/utils.ts
- sdk/packages/node/iii/src/types.ts
- sdk/packages/node/iii/tests/exports.test.ts
✅ Files skipped from review due to trivial changes (32)
- sdk/packages/rust/observability/README.md
- sdk/packages/rust/iii-example/Cargo.toml
- sdk/packages/node/iii/tsdown.config.ts
- sdk/packages/python/observability/README.md
- sdk/packages/node/iii/tests/payload.test.ts
- sdk/packages/node/observability/README.md
- sdk/packages/node/iii/src/iii.ts
- sdk/packages/node/iii/tests/otel-worker-gauges.test.ts
- crates/iii-worker/src/cli/worker_manager_daemon.rs
- sdk/packages/rust/iii-example/src/http_example.rs
- sdk/packages/python/iii/tests/test_context_propagation.py
- sdk/packages/rust/iii/README.md
- sdk/packages/node/iii/tests/baggage-span-processor.test.ts
- sdk/packages/python/iii-example/src/hooks.py
- sdk/packages/rust/iii/tests/payload_capture.rs
- sdk/packages/node/observability/tsconfig.json
- sdk/packages/rust/observability/src/telemetry/http_instrumentation.rs
- sdk/packages/rust/observability/src/logger.rs
- sdk/packages/python/iii/tests/test_trace_helpers.py
- sdk/packages/rust/iii-example/src/logger_example.rs
- sdk/packages/rust/observability/tests/payload_test.rs
- sdk/packages/python/iii/tests/test_telemetry_exporters.py
- pnpm-workspace.yaml
- docs/changelog/0-11-0/align-rust-register-function-with-signature.mdx
- crates/iii-worker/src/sandbox_daemon/mod.rs
- sdk/packages/python/iii-example/pyproject.toml
- engine/Cargo.toml
- sdk/packages/node/observability/tsdown.config.ts
- sdk/packages/python/iii/tests/test_baggage_span_processor.py
- sdk/packages/python/iii/tests/test_register_function_args.py
- sdk/packages/python/iii/src/iii/iii.py
- sdk/packages/rust/observability/tests/logger_test.rs
Removes the auto-render bump to docs/understanding-iii/triggers.mdx.skill.md so this PR carries no doc changes — the new SDK surface + observability notes are documented in PR #1686 (docs/0.14).
Per scope agreement, doc updates for the iii-observability extraction and the Rust register_function reshape ship via PR #1686 (docs/0.14) instead of this PR. Reverts every file under docs/ back to origin/main.
- connection.ts onFailed: isolate each callback in try/catch so one throwing handler doesn't block draining the rest. - http-instrumentation.ts executeTracedRequest: when input is a Request object, seed Headers from Request.headers first and let init.headers override. Previously the Request's own headers were silently dropped. - engine otel.rs: add BaggageSpanProcessor to the ExporterType::Both happy path so baggage propagation behavior matches the fallback branch regardless of OTLP exporter creation success. - skills/iii-rust-sdk/SKILL.md: bump install snippet from iii-sdk 0.10 to 0.13 (current package version). - skills/references/http-invoked-functions.rs: import Logger from iii_observability; iii_sdk no longer re-exports it.
…ity separately The iii-sdk crate no longer exposes an "otel" cargo feature — telemetry primitives moved to the iii-observability crate. Replace the features=["otel"] snippet with two plain version pins so users land on the correct dependency set.
guibeira
commented
May 25, 2026
| private doExport(spans: ReadableSpan[], resultCallback: (result: ExportResult) => void): void { | ||
| if (this.connection.getState() !== 'connected') { | ||
| const state = this.connection.getState() | ||
| if (state === 'failed') { |
Contributor
Author
There was a problem hiding this comment.
Why this file grew (commit 19e56b9f4)
Fixes shutdown deadlock: exporters queued spans while SharedEngineConnection wasn't 'connected' and waited for onConnected to drain. When state reached terminal 'failed' (WS retries exhausted), onConnected never fires → queued callbacks never run → BatchSpanProcessor.forceFlush() hangs 30s in teardown → vitest hookTimeout.
Added: onFailed hook drains queue with ExportResultCode.FAILED; doExport fast-fails when state already 'failed'. Same pattern in metric-exporter.ts + log-exporter.ts.
andersonleal
previously approved these changes
May 26, 2026
sergiofilhowz
previously approved these changes
May 26, 2026
# Conflicts: # skills/iii-rust-sdk/SKILL.md # skills/references/http-invoked-functions.rs
sergiofilhowz
approved these changes
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Why
Notes
Summary by CodeRabbit
New Features
Improvements